Hi @Itx-Psycho0. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Codecov Report

❌ Patch coverage is 78.51240% with 26 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.34%. Comparing base (e435a89) to head (af939b8).
⚠️ Report is 12 commits behind head on main.

@@            Coverage Diff             @@
##             main    #3728      +/-   ##
==========================================
+ Coverage   56.95%   57.34%   +0.39%     
==========================================
  Files         181      181              
  Lines       21116    21225     +109     
==========================================
+ Hits        12026    12172     +146     
+ Misses       7866     7823      -43     
- Partials     1224     1230       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

/ok-to-test

Review: feat: add TLS certificate support for Docker contexts

Branch: Itx-Psycho0/feat/docker-context-tls
Commit: 1af6e757b
Files: pkg/docker/docker_client.go, pkg/docker/docker_client_test.go

Overall

The approach of building the TLS config in-memory via newHttpClientFromContext() is sound. The test with a real mTLS mock daemon is solid. There are a few issues that should be addressed before merging.

Significant

1. Wrong fallback TLS path

docker_client.go — getDockerContextConfig(), around the fallback block:

// Docker stores context TLS files in contexts/meta/<sha256-hash>/
hash := sha256.Sum256([]byte(contexts[0].Name))
tlsPath = filepath.Join(dockerConfigDir, "contexts", "meta", fmt.Sprintf("%x", hash))

Docker stores TLS files under contexts/tls/<hash>/docker/, not contexts/meta/<hash>/. The meta directory holds meta.json metadata, not certificates. The comment is also incorrect.

The test masks this because it always sets Storage.TLSPath explicitly, so the fallback is never exercised. A test that omits Storage.TLSPath (or sets it to "<IN MEMORY>") and places certs at the correct fallback location would catch this.

2. Precedence inversion: context TLS overrides explicit env vars

docker_client.go — newHttpClient():

func newHttpClient() *http.Client {
	// First, try to get TLS config from Docker context
	if contextConfig := getDockerContextConfig(); contextConfig != nil && len(contextConfig.TLSCert) > 0 && len(contextConfig.TLSKey) > 0 {
		return newHttpClientFromContext(contextConfig)
	}

	// Fall back to environment variables
	tlsVerifyStr, tlsVerifyChanged := os.LookupEnv("DOCKER_TLS_VERIFY")
	...

Context is checked before DOCKER_TLS_VERIFY. If someone explicitly sets DOCKER_TLS_VERIFY=0 to disable TLS, but a Docker context with certs exists, context TLS will be used and the env var is never consulted. Convention is that env vars override config files, not the other way around. The env var check should come first, and context TLS should only be tried when DOCKER_TLS_VERIFY is not set.

3. Context TLS applied even when DOCKER_HOST is set manually

newHttpClient() is called unconditionally for all TCP connections, including when DOCKER_HOST was explicitly set by the user (not from context detection). If someone sets DOCKER_HOST=tcp://my-host:2376 without DOCKER_TLS_VERIFY, the code will now silently pick up TLS certs from whatever Docker context happens to be active. This mixes two independent configuration sources in a surprising way. Context TLS should only activate when the host itself came from context detection.

4. docker context inspect is executed twice

NewClient() calls GetDockerContextHostFunc() → getDockerContextHost() → getDockerContextConfig() (first subprocess). Then inside the isTCP branch, newHttpClient() calls getDockerContextConfig() again (second subprocess). Each call forks the docker CLI. The config should be fetched once and reused.

Minor

5. DockerContextConfig is exported but internal-only

The struct is only used within the docker package. It should be unexported (dockerContextConfig).

6. Redundant DOCKER_CONFIG passthrough

if dockerConfig := os.Getenv("DOCKER_CONFIG"); dockerConfig != "" {
    cmd.Env = append(os.Environ(), "DOCKER_CONFIG="+dockerConfig)
}

When cmd.Env is nil, the child process inherits the parent's full environment, which already includes DOCKER_CONFIG. This block just adds a duplicate entry and can be deleted.

7. getDockerContextHost() wrapper does unnecessary disk I/O

The backward-compat wrapper calls getDockerContextConfig() which reads cert files from disk, only to discard everything except .Host. When only the host is needed (the common case during host detection in NewClient), this is wasted I/O.

8. Silent error on malformed cert/key

In newHttpClientFromContext():

cert, err := tls.X509KeyPair(contextConfig.TLSCert, contextConfig.TLSKey)
if err == nil {
    ...
}

If X509KeyPair fails (malformed cert/key), the error is silently swallowed and the client is returned without client certificate auth. This will produce a confusing TLS handshake failure at connection time instead of a clear error at setup time. Consider returning an error or at least logging a warning.

Thanks for the detailed review @matejvasek! I see the issues, let me fix them:

1. Wrong TLS path - will change to contexts/tls/<hash>/
2. Precedence - will check env vars first, only use context if not set
3. Will only apply context TLS when host came from context detection
4. Will cache the config to avoid calling docker context inspect twice

Working on the fixes now!

Review: feat: add TLS certificate support for Docker contexts

Branch: Itx-Psycho0/feat/docker-context-tls
Commits: 1af6e757b, 1277ea5df
Files: pkg/docker/docker_client.go, pkg/docker/docker_client_test.go

Overall

Good iteration. The core approach — building TLS config in-memory via newHttpClientFromContext(), caching context config to avoid double subprocess spawns, and respecting env var precedence — is sound. Most issues from the previous round have been addressed. Two items remain before this is mergeable.

What was fixed

• Double subprocess spawn — fixed. contextConfig is fetched once in NewClient() and passed to newHttpClient(contextConfig).
• Wrong fallback TLS path — fixed. Now uses contexts/tls/<hash>/ with a corrected comment.
• Precedence inversion — fixed. newHttpClient() checks DOCKER_TLS_VERIFY first; context TLS is only tried when env vars are not set.
• Context TLS applied when DOCKER_HOST set manually — fixed. contextConfig is only populated when the host came from context detection; when DOCKER_HOST is set via env var, it stays nil.
• Exported type — fixed. dockerContextConfig is now unexported.
• Redundant DOCKER_CONFIG passthrough — fixed. Replaced with a comment.
• Silent error on bad cert/key — fixed. Now logs a warning to stderr.

Remaining issues

1. GetDockerContextHostFunc and getDockerContextHost() are now dead code

On upstream/main, NewClient() calls GetDockerContextHostFunc(). On this branch, NewClient() calls getDockerContextConfig() directly — GetDockerContextHostFunc is defined and exported but never called anywhere:

$ git grep 'GetDockerContextHostFunc' upstream/main
pkg/docker/docker_client.go: if contextHost := GetDockerContextHostFunc(); contextHost != "" {   # <-- used
pkg/docker/docker_client.go: var GetDockerContextHostFunc = getDockerContextHost

$ git grep 'GetDockerContextHostFunc' Itx-Psycho0/feat/docker-context-tls
pkg/docker/docker_client.go: var GetDockerContextHostFunc = getDockerContextHost                  # <-- defined but never called

This is a minor breaking change: any downstream code that mocked GetDockerContextHostFunc to inject test behavior will find their mock silently ignored. Two options:

• Remove GetDockerContextHostFunc and getDockerContextHost() entirely if no external consumers depend on them.
• Replace them with a GetDockerContextConfigFunc (or similar) mockable variable and use that in NewClient(), preserving the testability contract.

2. Test doesn't exercise the fallback TLS path

The fix to use contexts/tls/<hash>/ is correct, but the test always sets Storage.TLSPath explicitly via createDockerContextConfigWithTLS, so the fallback branch is never executed. Adding a test case where Storage.TLSPath is empty or "<IN MEMORY>" with certs placed at contexts/tls/<hash>/ would validate the fix actually works.

Nits

• The warning on stderr (fmt.Fprintf(os.Stderr, "Warning: ...")) works but log.Printf would be more conventional Go. Not blocking.

Also please add test for the previous TLS functionality using the envvars too.

Please add also test for the "old" functionality: TLS without context via the envvars.

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Itx-Psycho0
Once this PR has been reviewed and has the lgtm label, please ask for approval from matejvasek. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.